-
Notifications
You must be signed in to change notification settings - Fork 509
Fix empty call background color #4408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I see it I think that it should be a darker gray and the text should stay white.
Maybe var(--color-loading-dark)??
|
do we have a darker grey somewhere in the presets or should I invent a value ? |
eb17d94 to
e1ee2f2
Compare
marcoambrosini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Make the background grey and adjust text color to be readable. Signed-off-by: Vincent Petry <[email protected]>
e1ee2f2 to
4e50dfe
Compare
| } | ||
| h2, p { | ||
| color: var(--color-primary-text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want white, not primary text, otherwise you have black text on dark grey background if your primary theming color is bright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I hardcode white here ? then maybe I need to hardcode the background color as well instead of --color-loading-dark, if we agree that this is not something we want to theme.
as far as I can see here, only primary is defined: https://github.com/nextcloud/nextcloud-vue/blob/master/styleguide/assets/variables.css but if one would want to invert the colors they'd need to override "color-background-dark" with a light color ? or do we need a new primary-text-background color ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont want the screen to be blue or something so we should not use anything related to primary.
Also we dont want it to flip ot white with dark mode, so mostlikely the loading color is wrong too.
But can be tested easily, just search for "accessibility" in your dev nextcloud, turn on dark mode, search for theming and use "#ffffff" as primary color and then start a call.
Expected result is a dark screen with readable font color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/nextcloud/server/blob/master/apps/accessibility/css/dark.scss#L16
Dark mode makes loading bright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that whatever combination of variables I pick, it cannot look good in both dark mode and regular mode
so if we anyway don't want to invert the colors I'll hardcode white and dark grey in this view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> #4416

Make it the same grey like the stripe background and adjust text color
to be readable.
Fixes #4388